Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[10.x] Ensure array driver expires values at the expiry time #48497

Merged
merged 1 commit into from
Sep 22, 2023
Merged

[10.x] Ensure array driver expires values at the expiry time #48497

merged 1 commit into from
Sep 22, 2023
+60 −1

Conversation

timacdonald
Copy link
Member

@timacdonald timacdonald commented Sep 22, 2023 •
edited
Loading

The array cache driver's logic for determining when a key has expired is wrong.

It currently requires the time to be PAST the expiry - but the value indicates when the key expires at, thus if it is currently the time that it expires, it should have expired.

This means the array driver does not currently function like Redis. You can demo this yourself using the following script to test.

Make sure to test with array and with redis to see the difference.

You will notice that the array driver cannot make another hit until 1 second past the NEXT minute while Redis is able to make a hit at the very start of the second (give or take a few hundred milliseconds for communicating with Redis itself).

Artisan::command('limit', function () {
    while (now()->second !== 0) {
        echo '.';

        usleep(100_000);
    }

    echo PHP_EOL;

    $continue = true;

    while ($continue) {
        $continue = RateLimiter::attempt('echo-message', 10, function() {
            echo 'Executing rate limited code at '.now()->toDateTimeString().PHP_EOL;
        });
    }

    sleep(1);

    while (now()->second !== 0) {
        echo '.';

        usleep(100_000);
    }

    echo PHP_EOL;

    $continue = true;

    while (true) {
        $continue = RateLimiter::attempt('echo-message', 10, function() {
            echo 'Executing rate limited code at '.now()->toDateTimeString('m').PHP_EOL;
        });
    }
});

Array

Waiting until the start of the minute...
.............................................................................................................................................
.............................................................................................................................................
...............................................................
Executing rate limited code at 2023-09-22 05:56:00
Waiting until the start of the NEXT minute...
.............................................................................................................................................
.............................................................................................................................................
.............................................................................................................................................
.............................................................................................................................................
.........
Executing rate limited code at 2023-09-22 05:57:01.000

Redis

Waiting until the start of the minute...
.............................................................................................................................................
.............................................................................................................................................
..................................................................................................................................
Executing rate limited code at 2023-09-22 05:50:00
Waiting until the start of the NEXT minute...
.............................................................................................................................................
.............................................................................................................................................
.............................................................................................................................................
.............................................................................................................................................
.......
Executing rate limited code at 2023-09-22 05:51:00.054

The included test is one I was already writing that lead me down the path of testing all of this - so I left it in for us.

@timacdonald timacdonald marked this pull request as ready for review September 22, 2023 06:18
@timacdonald timacdonald mentioned this pull request Sep 22, 2023
Closed
@timacdonald timacdonald force-pushed the array-cache-expiry branch 2 times, most recently from 0c00c9f to 4c1dc79 Compare September 22, 2023 06:39
@timacdonald timacdonald marked this pull request as draft September 22, 2023 07:00
@timacdonald timacdonald marked this pull request as ready for review September 22, 2023 07:01
@taylorotwell taylorotwell merged commit ebb780c into laravel:10.x Sep 22, 2023
19 checks passed
@timacdonald timacdonald deleted the array-cache-expiry branch September 25, 2023 05:27
@rez1dent3 rez1dent3 mentioned this pull request Sep 27, 2023
Closed
@timacdonald timacdonald mentioned this pull request Sep 28, 2023
Merged
driesvints added a commit that referenced this pull request Sep 28, 2023
@driesvints
Revert "Fix array cache driver expiry (#48497)"
0bae014 
This reverts commit ebb780c.
@driesvints driesvints mentioned this pull request Sep 28, 2023
Merged
taylorotwell pushed a commit that referenced this pull request Sep 28, 2023
@driesvints
Revert expiry time changes (#48576)
cccfe05 
* Revert "Fix array cache driver expiry (#48497)"

This reverts commit ebb780c.

* Revert PR #48543
@rez1dent3 rez1dent3 mentioned this pull request Sep 28, 2023
Closed
@edcheyjr edcheyjr mentioned this pull request Oct 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@timacdonald @taylorotwell